-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[jsk_pcl_ros_utils][polygon_magnifier] Allow negative param value to shrink #2053
Conversation
330c27b
to
44a78a8
Compare
|
||
ret_polygon_array.likelihood.reserve(msg->likelihood.size()); | ||
std::copy(msg->likelihood.begin(), msg->likelihood.end(), | ||
std::back_inserter(ret_polygon_array.likelihood)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@furushchev Could you please explain what these lines do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkentaro Thanks for reviewing! What is done in this part is just to copy all elements of vector. Original code drops labels
and likelihood
from original message even if they have some values.
ref:
fixed a bug to drop labels and likelihood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you can't firstly copy all elements from input msg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkentaro Because I didn't know the way. Updated.
44a78a8
to
3428d49
Compare
@wkentaro Thanks for very kind tutorials! Updated. |
new_vertices[i] = (vertices_[i] - c).normalized() * distance + vertices_[i]; | ||
Eigen::Vector3f diff_vec; | ||
if (distance > 0.0) diff_vec = vertices_[i] - c; | ||
else diff_vec = c - vertices_[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
distance is not used in this if block.
could u explain why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkentaro instead abs(distance)
is multiplied. Original code may make polygons which has opposite directed normals. (I could not know why though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abs(distance)
is out of if block, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean why the sign of distance decides the sign of diff_vec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkentaro Please read the code, if distance
is negative, we need to compute a vector of opposite direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked @mmurooka for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked with @wkentaro ,
and this change is not necessary (i.e. this does not change behavior at all).
Because if distance is negative
diff_vec
is c - vertices_[i]
, and diff_vec * abs(distance)
becames diff_vec * (- distance) = (vertices_[i] - c) * distance
.
This is same result with original source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abs returns integer. please use fabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part was reverted and no abs
is used any more. Thanks anyway.
@@ -0,0 +1,65 @@ | |||
#!/usr/bin/env python | |||
# -*- coding: utf-8 -*- | |||
# Copyright: Yuki Furuta <furushchev@jsk.imi.i.u-tokyo.ac.jp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add license. BSD, MIT, or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I could not find other nodes that have license header.
Even if this is preferred, I think we should create another PR to update it for all these nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other nodes, the license is estimated by package.xml (probably BSD), but you specify copyright, and I thought it was under another license.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you write Author: ...
, I can ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this line is automatically added by emacs. Removed.
0402cbf
to
0c7c2bd
Compare
Yes, but result is different.(in original code, normal direction of plane
is sometimes flipped) I don't know why. Do you know.??
I'll give you rosbag if you want to see the behavior.
|
def publish(self, event): | ||
# update timestamp | ||
self.msg.header.stamp = event.current_real | ||
for p in self.msg.polygons: p.header.stamp = event.current_real |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really?
# validate params | ||
assert isinstance(param, list), "polygons must be list" | ||
|
||
has_label = any(map(lambda p: "label" in p, param)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_label = any('label' in p for p in param)
assert isinstance(param, list), "polygons must be list" | ||
|
||
has_label = any(map(lambda p: "label" in p, param)) | ||
has_likelihood = any(map(lambda p: "likelihood" in p, param)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_likelihood = any('likelihood' in p for p in param)
for polygon in param: | ||
ps = PolygonStamped() | ||
ps.header.frame_id = frame_id | ||
ps.polygon.points = map(lambda v: Point32(v[0], v[1], v[2]), polygon["points"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Point32(*v) for v in polygon['points']]
I don't know. Please describe |
I executed sample_polygon_magnifier.launch with this PR update, and it worked (normal was not flipped).
to origin/master, then build and executed same sample launch, and it also worked (normal was not flipped). |
Sorry for many reply comments. This can happen regardless of your change in |
master...mmurooka:polygon-magnifier-normal-flip reproduce normal flipping. |
@mmurooka You are totally right!! Thanks! I'll write a code to warn if breaks original convex hull.
Sorry, if I understand correctly, you mean |
= poly.magnifyByDistance(magnify_distance_); | ||
|
||
if (!magnified_poly->isConvex()) { | ||
ROS_ERROR("Magnified polygon %ld is not convex.", i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is OK to push magnified polygon even if it is non-convex. In some cases, we want the result even if it is non-convex. Keeping length of polygon array is easy to manage.
How about following (just add warn message if non-convex)?
jsk_recognition_msgs::PolygonArray ret_polygon_array = *msg;
for (size_t i = 0; i < msg->polygons.size(); i++) {
jsk_recognition_utils::ConvexPolygon poly = jsk_recognition_utils::ConvexPolygon::fromROSMsg(msg->polygons[i].polygon);
jsk_recognition_utils::ConvexPolygon::Ptr magnified_poly = poly.magnifyByDistance(magnify_distance_);
if (!magnified_poly->isConvex()) {
ROS_WARN("Magnified polygon %ld is not convex.", i);
}
ret_polygon_array.polygons[i].polygon = magnified_poly->toROSMsg();
}
pub_.publish(ret_polygon_array);
I'm saying about this figure in this comment. https://github.com/furushchev/jsk_recognition/blob/69b0e4c079bc35200d0862cd8be400252899c15a/doc/jsk_pcl_ros_utils/nodes/images/polygon_magnifier.png |
If keeping convex (and not flipping normal) is important for your use case, I recommend you to use magnify by scale in nodelet, and add rosparam to select magnifying by distance or scale. |
69b0e4c
to
05920be
Compare
@mmurooka Thanks for advice, updated / rebased and yes, I'll send another pull request to support |
LGTM! |
def publish(self, event): | ||
# update timestamp | ||
self.msg.header.stamp = event.current_real | ||
for p in self.msg.polygons: p.header.stamp = event.current_real |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really?
@@ -14,4 +17,10 @@ Magnify polygons by specified length. | |||
## Parameters | |||
* `~magnify_distance` (Double, default: `0.2`) | |||
|
|||
Length to scale polygon | |||
Length to scale polygons. Default value `0.2` means the distance of each corresponding edges will be `0.2` m. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please describe that this nodelet supports negative value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry I added newline in different line.
…nify [jsk_pcl_ros_utils][polygon_magnifier] update docs [jsk_recognition_utils] add polygon_array_publisher.py / sample_polygon_array_publisher.launch [jsk_pcl_ros_utils] add sample / test for polygon_magnifier
05920be
to
2a7b621
Compare
Hmm, failed on indigo... 20:29:28 Could NOT find Cython (missing: CYTHON_EXECUTABLE)
20:29:28 Call Stack (most recent call first):
20:29:28 /usr/share/cmake-2.8/Modules/FindPackageHandleStandardArgs.cmake:315 (_FPHSA_FAILURE_MESSAGE)
20:29:28 cmake/FindCython.cmake:42 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
20:29:28 cmake/UseCython.cmake:74 (find_package)
20:29:28 CMakeLists.txt:47 (include) |
OK. Test passed! |
magnify_distance
param to shrink polygons.labels
andlikelihood
.